Document what physical_key schemes and timestamp tags actually mean - #818

Comments only — no behavior change. just lint clean, cargo doc -p quilt-rs clean.

These four comments record behavior that surprises, found while tracing whether a manifest row's physical_key scheme could serve as a local-vs-remote origin signal for a package-list recency view. It cannot, and none of the reasons were written down.

flow::install_paths

The doc comment claimed "installed paths have place pointing to file://location". They don't. Installed rows keep the remote s3:// key, and that's correct: physical_key names where the canonical bytes live, and keeping the remote key is what lets a later push skip the re-upload (matches_content ignores physical_key, so use_existing_row_or_upload copies the remote key across on a content match).

Three related fixes in that function:

• the doc comment now describes actual behavior and why it's right;
• the place value in the loop is marked diagnostic-only — it is computed, logged, and discarded (the error arm still works as an absolute-path assertion);
• the plan sketch's "replace entry's physical key in the manifest with the cached physical key" step is marked as never implemented.

It also records the conclusion that motivated the trace: a file:// key means "these bytes were committed locally" — written by flow::commit and flow::create — and must not be read as a per-row origin marker.

flow::push::push_package_impl

The uploaded manifest is built into the cached manifests dir, and the success path never copies it over the installed one — copy_cached_to_installed runs only in the hash-mismatch error branch. So a locally committed row keeps its file:// key in .quilt/installed/ after a successful push, clearing only incidentally when a later pull or reset refreshes the installed manifest.

That divergence is safe because top_hash excludes physical_key (manifest::top_hasher hashes hash, logical_key, meta, size), so both copies are the same revision — which is also why the new_manifest_uri.hash != commit.hash guard doesn't fire on a normal push.

io::manifest::upload_row

The file:// scheme check is load-bearing, not defensive. Together with use_existing_row_or_upload's content-match branch it's what keeps a local key out of a published manifest: every row of an uploaded manifest goes through one of those two branches, so the key is either replaced with the S3 destination or came from the remote to begin with. It also closes the reverse leak — an already-s3:// row whose content doesn't match the destination hard-errors instead of publishing a manifest that addresses another bucket's objects.

io::manifest::tag_timestamp

.quilt/named_packages/<ns>/<epoch> is the registry's only record of revision times — the manifest format carries no timestamps in either the header or the rows. It's write-only from this client: reading revision times back means enumerating those tag objects, and the Remote trait has no list operation. Also noted next to the existing collision TODO that the tag key holds the commit time at one-second resolution, so two commits of the same namespace made within one second collide even if they're pushed hours apart — upload_tag's unconditional put means the second push silently overwrites the first's tag.

Review pass (force-pushed): corrected two inaccurate claims from the first version — file:// keys are written by create_package as well as commit, and the tag collision window is governed by commit time, not push time — dropped a non-exhaustive enumeration of Remote's methods, disambiguated the super::commit intra-doc link, and tightened all four comments (72 → 59 added lines).

🤖 Generated with Claude Code

Greptile Summary

This PR documents existing manifest-key and timestamp-tag behavior without changing executable behavior.

• Clarifies that installed manifest rows preserve their existing physical_key.
• Explains why successful pushes can leave installed and cached manifests with different physical-key schemes.
• Documents upload-row scheme handling and timestamp-tag limitations.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

_{Reviews (2): Last reviewed commit: "Correct five inaccurate claims in the ph..." | Re-trigger Greptile}

Context used:

• Knowledge Base — Quilt-rs flow layer
• Knowledge Base — quilt-rs io subsystem